Skip to content

feat: Add sagemaker_image and sagemaker_image_info - #54

Open
JanLikar wants to merge 9 commits into
ansible-collections:mainfrom
xlab-si:feat/sagemaker_image
Open

feat: Add sagemaker_image and sagemaker_image_info#54
JanLikar wants to merge 9 commits into
ansible-collections:mainfrom
xlab-si:feat/sagemaker_image

Conversation

@JanLikar

Copy link
Copy Markdown
SUMMARY

Add sagemaker_image, sagemaker_image_info and associated integration tests.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

sagemaker_image
sagemaker_image_info

ADDITIONAL INFORMATION

Assisted-by: Claude Sonnet 5

@GomathiselviS GomathiselviS left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting the PR. Great work! Added a few comments.

Comment thread plugins/modules/sagemaker_image.py Outdated
Comment thread plugins/modules/sagemaker_image.py
Comment thread plugins/modules/sagemaker_image.py Outdated
Comment thread plugins/modules/sagemaker_image.py Outdated
Comment thread plugins/modules/sagemaker_image_info.py Outdated
@JanLikar
JanLikar requested a review from GomathiselviS August 19, 2026 09:47
@oraNod

oraNod commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Code Review Findings

1. delete_image doesn't wait for completion (sagemaker_image.py:283)
delete_image returns immediately without honoring the wait parameter (default true). SageMaker's DeleteImage is async, so chaining a create after delete can race against the ongoing deletion and fail.

2. Docs/code mismatch on wait for delete (sagemaker_image_module.rst:366)
The RST docs say wait applies to "create, update, or delete" but the module source and DOCUMENTATION string only say "create or update". Either add wait-on-delete support or fix the docs.

3. Info module fails on missing image (sagemaker_image_info.py:145)
sagemaker_image_info calls fail_json when an image isn't found. Standard Ansible _info convention (and sibling sagemaker_code_repository_info) returns an empty list instead, which supports lookup-before-act patterns without ignore_errors.

4. Unnecessary list_tags API call (sagemaker_image.py:256)
update_image always calls list_tags even when the user didn't pass tags. Guard with if module.params.get("tags") is not None to skip the wasted roundtrip.

@oraNod

oraNod commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Thank you for the PR @JanLikar ! I'm somewhat new to this team and not as familiar with sagemaker as others so I conducted a review with Claude. I posted those findings in my previous comment. I hope they are helpful!

Comment thread plugins/modules/sagemaker_image_info.py Outdated
Comment thread plugins/modules/sagemaker_image_info.py
Comment thread plugins/modules/sagemaker_image_info.py Outdated
Comment thread plugins/modules/sagemaker_image_info.py Outdated
Comment thread plugins/modules/sagemaker_image.py
Comment thread plugins/modules/sagemaker_image.py
Comment thread plugins/modules/sagemaker_image.py
@oraNod

oraNod commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Follow-up: image_deleted waiter workaround

Reviewed the upstream issue (boto/botocore#3782) and the reproduction gist. The root cause is clear: botocore's ImageDeleted waiter expects ResourceNotFoundException but SageMaker returns ResourceNotFound. The custom _wait_for_image_deletion polling loop is the right approach here.

The implementation in the latest push looks good — one suggestion for parity with the upstream waiter:

Check for DELETE_FAILED during polling. The botocore waiter treats DELETE_FAILED as a failure condition, but the custom loop doesn't — it would keep polling until timeout. Consider adding an early exit:

def _wait_for_image_deletion(
    client,
    module: AnsibleAWSModule,
    image_name: str,
    delay: int,
    max_attempts: int,
) -> None:
    for attempt in range(max_attempts):
        try:
            image = describe_image(client, image_name)
            if image is None:
                return
            if image.get("ImageStatus") == "DELETE_FAILED":
                module.fail_json(
                    msg=f"SageMaker Image {image_name} entered DELETE_FAILED state.",
                )
        except (
            is_boto3_error_code("ResourceNotFound"),
            is_boto3_error_code("ResourceNotFoundException"),
        ):
            return

        if attempt < max_attempts - 1:
            time.sleep(delay)

    module.fail_json(
        msg=(
            f"Timeout waiting for SageMaker Image {image_name} to be deleted. "
            "Polling did not confirm deletion within the timeout period."
        )
    )

Minor note: since describe_image already catches ResourceNotFound and returns None, the except block here would only fire for ResourceNotFoundException — which is defensive but harmless. When botocore ships the upstream fix, this function can be removed and the standard waiter path in _wait_for_image_status will work.

Comment thread docs/amazon.ai.sagemaker_image_info_module.rst
Comment thread docs/amazon.ai.sagemaker_image_info_module.rst
Comment thread docs/amazon.ai.sagemaker_image_info_module.rst
Comment thread docs/amazon.ai.sagemaker_image_info_module.rst
Comment thread plugins/modules/sagemaker_image.py
Comment thread plugins/modules/sagemaker_image.py
Comment thread plugins/modules/sagemaker_image.py
Comment thread plugins/modules/sagemaker_image_info.py
Comment thread plugins/modules/sagemaker_image_info.py Outdated
Comment thread plugins/modules/sagemaker_image_info.py Outdated
Comment thread plugins/modules/sagemaker_image.py
JanLikar and others added 2 commits August 20, 2026 14:53
Co-authored-by: Don Naro <dnaro@redhat.com>
@sonarqubecloud

Copy link
Copy Markdown

@oraNod oraNod left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JanLikar 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants